Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add CType block number support #694

Merged
merged 26 commits into from
Jan 25, 2023
Merged

Conversation

ntn-x2
Copy link
Member

@ntn-x2 ntn-x2 commented Dec 1, 2022

Fixes https://github.com/KILTprotocol/ticket/issues/2330.

It follows a similar process as to what was done for public credentials, and it indeed also refactor some common code into a utils.ts module within the core package, which is not exported outside.

I cannot be merged before KILTprotocol/kilt-node#440 is merged in the node codebase, but it can be reviewed nonetheless.

Checklist

@ntn-x2 ntn-x2 self-assigned this Dec 1, 2022
@ntn-x2 ntn-x2 force-pushed the aa/ctype-blocknumber-support branch from 54ef85f to c41855a Compare December 14, 2022 10:05
@ntn-x2 ntn-x2 added the ✋on hold status: on hold label Dec 14, 2022
@ntn-x2 ntn-x2 requested review from rflechtner and arty-name and removed request for rflechtner December 14, 2022 10:09
@ntn-x2 ntn-x2 marked this pull request as ready for review December 14, 2022 10:09
@ntn-x2 ntn-x2 changed the base branch from develop to aa/update-polkadot-dependencies December 14, 2022 10:09
Copy link
Collaborator

@arty-name arty-name left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable

packages/core/src/ctype/CType.chain.ts Outdated Show resolved Hide resolved
packages/core/src/ctype/CType.chain.ts Show resolved Hide resolved
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would argue that CType.fromChain is not following the original convention any more, according to which fromChain functions are decoders. I'd say we rename it and remove the necessity of calling api.query.ctype.ctypes before calling it (as it performs state reads anyway). I'm undecided whether the original fromChain should be kept, which would simply return a block number and ctype owner.

*/
export function fromChain(encoded: Option<AccountId>): DidUri {
return Did.fromChain(encoded.unwrap())
export async function fromChain(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This used to be a simple decoder (which frankly I think we didn't really need). Given this huge extension in functionality, I'm in favour of replacing this with something like lookupFromChain, which does what this does but additionally calls api.query.ctype.ctypes internally on the hash extracted from the ctype id.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also thought about it, and wanted to do the same for the public credentials, since we do something very similar. In my opinion, we should get rid of the need to call api.query or api.call for those places that need to do something else with the result anyway, namely ctypes and public credentials. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as they are comparable, yes, that would be my preference. I know very little about how it's done with the public credentials though

@arty-name
Copy link
Collaborator

I agree that the code doesn’t really fit the name fromChain. Trying to recall other cases like this in the SDK, I realized that some places were simplified by moving the complexity into the Rust code of RPC calls. Would it make sense to do the same here?

@rflechtner
Copy link
Contributor

If we have a similar case with the public credentials, I'd agree it would be good to rename/change that as well.
On the issue of rpc calls to move these operations to node client, I'm definitely a fan because the nodes tend to be upgraded on runtime changes, so if anything changes storage wise applications don't need to go through the hassle of trying to update at the right time to avoid breaking

@ntn-x2
Copy link
Member Author

ntn-x2 commented Dec 15, 2022

I see your point guys, but I also think that considering changing the approach we use for block-based information retrieval is out of scope of this PR. Definitely having an RPC endpoint that clients can call could be beneficial, but we need to remember that RPC methods and types need to be manually upgraded anyway whenever the client is updated, which I would argue is even less "clear" than a runtime update since there could be nodes running different versions of the client software, and since the polkadot library does not support versioning for RPC calls, we would have the same issue of "forcing" node operators to upgrade anyway.
I agree there could be a better way of retrieving this stuff, but what about we merge this, and discuss an alternative strategy that could be applied to public credentials (not touched here), ctypes, and other things in the future?

@ntn-x2
Copy link
Member Author

ntn-x2 commented Dec 15, 2022

In the end, this approach, albeit not the optimal, is still better than no approach, and we are not releasing it right away today.

@rflechtner
Copy link
Contributor

Oh yes, i did not want to say that this would keep us from merging the sdk support as it is right now.

@arty-name
Copy link
Collaborator

Do you still want to include the renaming of the function in this PR?

@ntn-x2
Copy link
Member Author

ntn-x2 commented Dec 15, 2022

@arty-name For public credentials, we call the methods credentialFromChain and credentialsFromChain, and I remember that we used that because although they come from a runtime API the user should not know that, and still use the same method as elsewhere (could not find a comment for it, must have happened via some other channels). Now I feel I am getting the opposite feedback, where we should chain the fromChain naming because it does not come from chain state? What would be your suggestion?

@arty-name
Copy link
Collaborator

When I was looking at these PRs I didn’t properly consider the naming convention and everything looked fine to me. Now @rflechtner has raised this topic, and I can see what he means, while I still don’t have a strong opinion. Then I felt that the point was forgotten about in the discussion, and wanted to clarify whether Raphael still thinks it should be done. I believe it’s his call and I will be fine with both outcomes.

@rflechtner
Copy link
Contributor

rflechtner commented Dec 15, 2022

Ok summing up to avoid confusion:

  • please rename fromChain in this PR to something along the lines of fetchFromChain. Inside this function call api.query.ctype.ctypes on the ctype hash taken from the id.
  • if these are comparable, align the behaviour of credential(s)FromChain with the above
  • reintroduce a fromChain function on CType.chain.ts which acts as a simple decoder, returning {owner, blockNumber}.
  • make a ticket for future investigation of rpcs for locating public credentials & ctypes

@ntn-x2
Copy link
Member Author

ntn-x2 commented Dec 16, 2022

@rflechtner I addressed your points 1, 2, and 3 in the commits above. In general, I think that mentioning the use of the api.query function when we have api.call is not a good thing. If it would have been possible, we would have hidden the query stuff altogether and simply allow users to use the runtime APIs. Now we have fromChain functions that are meant to be used in conjunction with the query stuff, when the query stuff only provide a partial picture of the data that the user most likely needs. Then why do we decorate the runtime API? I also feel we are re-wrapping functions inside other functions for ease of use, which I personally think goes against the way the SDK was supposed to be used.
This said, as I am definitely less involved than you and @arty-name in the SDK, I will accommodate any suggestions you make, as I have already done in the commits above, but I wanted to express my point of view.

Regarding point 4, we do have a ticket for it, but I don't think it is actually a scalable approach. Now we are putting more load on the server, which is a single instance, rather than putting the parsing load on the client, which are distributed. This will have to be considered carefully, as if we have, potentially thousands of credentials which we want to filter by, say, attester, the node would have to do that once for each client request, while the client could fetch all the data it needs and perform the filtering on its side.

Base automatically changed from aa/update-polkadot-dependencies to develop December 16, 2022 10:33
Copy link
Contributor

@rflechtner rflechtner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides some small issues in the docstrings this looks great!

Concerning your remarks, we can have decoders for runtime calls as well; I don't think these are exclusive to state db queries. The convention we introduced simply says that if a function is called *fromChain() it's a simple decoder that transforms a SCALE encoded value into a JSON-like object.

I'm not entirely sure what you're referring to with the wrapping of functions for convenience? If you were talking about the decoders, I would be happy to not have them at all, but could also see that it may get tricky to understand how certain on-chain data is decoded, especially when using js or where e.g. account ids get transformed to URIs/DIDs. I assume it's not fetchFromChain and its publicCredentials cousin, is it? I'd agree we're moving away from what we originally wanted for the sdk with these, but given their complexity writing a recipe instead is just not an option. That's one of the reasons I was pushing for moving this logic to the node side, next to the side effect that this reduces traffic and may help in minimising breaking changes that applications need to try and keep up with.

Side note: I'm not sure I agree that we must try to minimise load on rpc nodes and treat them as 'servers'; should the rpc node not be part of the client-side setup wherever this is feasible (i.e. outside of mobile & browser environments)?

packages/core/src/ctype/CType.chain.ts Outdated Show resolved Hide resolved
packages/core/src/ctype/CType.chain.ts Outdated Show resolved Hide resolved
packages/core/src/ctype/CType.chain.ts Show resolved Hide resolved
@ntn-x2 ntn-x2 removed the ✋on hold status: on hold label Jan 24, 2023
@ntn-x2 ntn-x2 merged commit 100079b into develop Jan 25, 2023
@ntn-x2 ntn-x2 deleted the aa/ctype-blocknumber-support branch January 25, 2023 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants